-
-
Notifications
You must be signed in to change notification settings - Fork 645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft: specifying default connection params #3359
base: master
Are you sure you want to change the base?
Conversation
Couple of downsides / tradeoffs with the above:
Note: in the above scenario, if the port 2171 doesn't exist, the command throws a "No server found" error - which I very much prefer over being presented with a empty completion list, and having to take a second or two to interpret it. ("Oh, I haven't started the server yet, and Cider couldn't detect any port files, that's why I'm seeing a lack of something") |
So this particular implementation suits our workflow where each project's dev REPL has its own "identifiable" static port number, instead of being assigned a random one by cider-nrepl. Port clashes are a feature because we don't want two simultaneous dev servers of the same project started by accident. Might it be reasonable to assume similar practices for users who choose to |
That'd be fine by me. I prefer explicit errors. A general note - If I recall correctly CIDER currently extracts the port number from the |
I was thinking, maybe given that cider-connect-clj already accepts This would save us the few possible complications detected in the discussions (GH + Slack) |
I still think that's not a big deal, provided there's an easy way to ignore the default connection params. And most of the time you'd want to connect automatically to some nREPL endpoint, so that'd be a productivity improvement IMO (although a small one). |
I think this PR implements something like it - use a prefix arg / C-u before Feel free to take or adapt the changes here in any way for the Clojurescript side of things - as mentioned on the Slack this PR was just a code reference for a "Can CIDER do this?" question that I've already hacked together on a local fork. (From my understanding the advice mechanism is mostly a last resort for modifying external packages out of our control, probably shouldn't be used in this case) |
5036392
to
51e84e7
Compare
Resolved conflicts with the master branch - any interest in this PR and/or generalizing it to work with CLJS commands? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any interest in this PR and/or generalizing it to work with CLJS commands?
Sure, let's go for it. Having cljs from the beginning would seem important, else it could get confusing/broken shortly thereafter.
Thanks!
cider.el
Outdated
@@ -1504,14 +1504,21 @@ server buffer, in which case a new session for that server is created." | |||
(plist-put :session-name ses-name) | |||
(plist-put :repl-type 'cljs))))) | |||
|
|||
(defvar-local cider-connect-default-params nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to have two variables - cider-connect-default-clj-params
, cider-connect-default-cljs-params
.
Commands like cider-connect-clj&cljs
would use the former for the clj part, the latter for the cljs part (which would seem cleaner than a third variable cider-connect-default-clj&cljs-params
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(replied in the main thread below by accident, feel free to continue conversation here)
As far as I can tell, the only difference in params between I'm not familiar with clojurescript dev workflows, maybe there are instances where one would have different nREPL hosts / ports for CLJ and CLJS repls? (noting that the clj&cljs command reuses the same host and port for the sibling cljs repl without double-prompting) |
It's reasonably frequent to have different ports. There's also the case of babashka (we sometimes use 'cljs' to denote 'bb'), where definitely there are different ports being used for the vanilla JVM and the bb process. Either way, I'd try to not be too smart here - leaving everyone with something easy to understand/maintain. ...I especially wonder about case of accidentally connecting with cljs params to a clj repl or vice versa. Potentially very confusing.
Good catch! It seems like the right thing would be to use the If both vars are set, honor them individually, respectively. |
That makes sense to me as well. |
Hmm, it appears that there's an existing defcustom
It's not explicitly mentioned, but This points to a possible conflict - what would be the desired outcome of the following dir-locals.el? ((clojure-mode
(cider-connect-default-cljs-params . (:host "localhost" :port 8000 :cljs-repl-type figwheel))
(cider-default-cljs-repl . shadow)))) |
Good eye!
Setting defcustoms from .dir-locals is also slightly fragile - in the end it's a global side-effect with no guarantees of being wiped as one switches dirs. So it seems reasonable to let the new var win. |
I'm not sure what you mean by this. Everything set via And I agree that probably the connection params should override the default REPL setting. |
I wasn't aware of this, could you help me understand where such a situation would occur? I'm familiar with using Babashka as a scripting language, but this involves |
I think it's actually the other way around, as some ClojureScript platforms (e.g. |
True (except for the case of wrapping setq with eval - but should be rare) Fairly certain of a case of 'cljs' meaning 'bb' - will check later today. I think it was related to the repl-type. |
But regardless, one should be able to have two independent connections for any combination of So having two 'slots' that are known to be independent (not merged, assoced, dissoced) would only help. |
Please correct me if I'm mistaken, but does this mean that Cider's notion of a |
Probably. For full-stack-Clojure apps using shadow-cljs, the most common setup would be to spin up different JVMs for the backend and for the frontend. (I'd enjoy the most having a single JVM, but last time I asked, going over npx is the most supported route) Which is to mean, things have changed, not only with bb and friends but also within the cljs ecosystem. (another cljs thing that is less relevant today is Piggyback) |
Stepping back for a moment, the motivation here is really just convenience - the user has some known external means of launching one or more REPLs, at some fixed TCP ports / sockets, and wants some means to inform Cider of this fact in order to skip the small annoyance of pressing RET twice on every cider-connect. We see the same principle in existing defcustoms like This PR simply extends this to the "Select host" and "Select port" prompts for a
Thanks for this pointer - perhaps I should clone a simple starter shadow-cljs app to better understand how this would work. Say the user executes (in a clj buffer) The goal here is simply to reduce the number of RET's required from the user, via the minimal customization spec in the dir-locals needed to inform CIDER of their intention. |
Yes, but adding that convenience shouldn't introduce or amplify coupling, assumptions, etc. In the end, I believe the provided feedback is very simple to implement. It's also very easy for users to use. Data, unlike code, tends to be cheap to duplicate with no consequences. In specific terms, I much prefer that users have to specify the port twice in .dir-locals.el (it's data, it's cheap) than having us couple things here (it's code, it's expensive). Re: shadow-cljs what I consider a typical full-stack workflow is:
So we have two JVMs in total. Not all Clojure apps are full-stack-Clojure, and even then, there are workarounds (like letting shadow-cljs manage the backend JVM s well) for keeping it single-JVM. tldr there are many scenarios. By keeping things decoupled, all use cases fit into the equation. |
Sure, that sounds reasonable :) On an (even more meta) preference level, I feel like CIDER already suffers from a sort of proliferation too many overlapping and confusingly-named customization options, thus I was a little hesitant about adding to it, eg. by carelessly introducing variant -clj/-cljs defcustoms when a single one would suffice. In this case there appear to be good reasons for decoupling them, thanks for spending the time to clarify why 🙇 Re. naming, how about
|
I also considered putting "default" first, in line with
But the first pair makes it clearer that they apply only to the We could also omit it altogether, in line with many other defcustoms like
Or even use "preferred" like in
|
I appreciate the receptiveness and patience 🙌 I'll probably be fine with any naming that you find suitable. |
Oh and separately, we should note that there's an existing family of defcustoms named These are specifically related to a jack-in based workflow and have no effect on I imagine a possible confusion could arise when a user wants to specify a fixed port while jacking in (eg. so she can dependably connect from another process which has no access to the generated Whether or not they should is a trickier matter (and probably out of scope for this PR), but might affect the naming if we decide to tackle it in the future. |
SGTM, so indeed making |
51e84e7
to
46007a0
Compare
46007a0
to
803c7fc
Compare
I've rebased onto master and pushed a change for |
Another note: I discovered this other variable |
Thanks for pushing through this!
Regardless yes, it could plausibly be extended to satisfy current needs. I'd say, feel free to explore in that direction. If it's too hard to keep using it with extended semantics in a non-breaking manner, we could also favor this new PR. |
You wrote in Slack:
Perhaps the 'globality' is enough of a reason to deprecate the var. Most likely changing the var type would be a breaking change. |
Agreed. I guess we had mostly forgotten about it, but it should be hard to add the newer params to it.
It wasn't meant to be used in |
Context: https://clojurians.slack.com/archives/C0617A8PQ/p1687640686417379
As discussed on Slack, a naive clj-only implementation of a feature which allows the user to skip
cider-connect
prompts asking for the hostname and port.The intended usage here is to set
cider-default-connect-params
explicitly per project as a dir-local variable, which is why it's adefvar-local
instead ofdefcustom
.Eg. I have a project which specifies in its deps.edn
so running
clj -M:dev
in an external terminal always starts a Cider nrepl server on the port 2171.Then in the project's
.dir-locals.el
:Result:
M-x cider-connect
on any clj buffer within that project skips the prompts entirely, saving the friction of a couple ofRET RET
keypresses on every connect. Which can be quite frequent when investigating project crashes (or hacking on Cider itself)Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
eldev test
)eldev lint
) which is based onelisp-lint
and includescheckdoc
, check-declare, packaging metadata, indentation, and trailing whitespace checks.